Skip to content
This repository was archived by the owner on Feb 5, 2025. It is now read-only.

Issues/9618 login with username #42

Merged
merged 5 commits into from
Jan 12, 2019
Merged

Issues/9618 login with username #42

merged 5 commits into from
Jan 12, 2019

Conversation

@aerych
Copy link
Contributor

@aerych aerych commented Jan 11, 2019 •
edited by mindgraffiti
Loading

Refs: wordpress-mobile/WordPress-iOS#9618

This PR adds custom handling for the email_login_not_allowed error that is sometimes occurs when logging into WordPress.com when a user's email address meets the criteria for being "suspicious". In these cases we take the user to the LoginSelfHostedViewController, but configured to log into WordPress.com as the site. This allows the user to proceed with their username/password credentials rather than being stuck on the email address screen without a clear path forward.

With these changes we:

  • check for the error in question
  • assign the error to be displayed
  • show the LoginSelfHostedViewController via a newly added segue
  • show the error after presenting the new view controller to provide context for what to do next

Example at C02Q9GACF/p1547220737194300-slack-ios-osx-dev | p1547234802205800-slack-C02Q9GACF

Pinging both @mindgraffiti and @nheagy for sanity checks since this is a shared library.

@aerych aerych added the enhancement New feature or request label Jan 11, 2019
@aerych aerych requested review from mindgraffiti and nheagy January 11, 2019 15:49
This was referenced Jan 11, 2019
Closed
Closed
mindgraffiti
mindgraffiti approved these changes Jan 11, 2019
View reviewed changes
Copy link
Contributor

@mindgraffiti mindgraffiti left a comment •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

For WCiOS, it displays the correct error message. Hitting "okay" dismisses the popup but does not present the username / password VC. I pretty sure this is by design, as we have the self-hosted login flow turned off right now.

WordPressAuthenticator/Signin/LoginEmailViewController.swift Outdated
/// error message when the new view controller appears.
///
@objc func showSelfHostedUsernamePasswordAndError(_ error: Error) {
loginFields.siteAddress = "http://wordpress.com"
Copy link
Contributor

@mindgraffiti mindgraffiti Jan 11, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nitpicky: this should use https://wordpress.com

@mindgraffiti mindgraffiti mentioned this pull request Jan 11, 2019
Closed
nheagy
nheagy approved these changes Jan 12, 2019
View reviewed changes
Copy link
Contributor

@nheagy nheagy left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think this one is good to go 😀

:shipit:

@aerych
Copy link
Contributor Author

aerych commented Jan 12, 2019

Thanks @nheagy and @mindgraffiti !!

@aerych aerych merged commit 3db5f20 into develop Jan 12, 2019
@aerych aerych deleted the issues/9618-login-with-username branch January 12, 2019 01:36
@AmandaRiu AmandaRiu mentioned this pull request Mar 29, 2019
Closed
@hypest
Copy link

hypest commented Apr 10, 2019

👋, I'm trying to investigate and fix the issue for the Android side and I'm having trouble convincing the server to send the email_login_not_allowed error type.

I wonder, were you able to verify that the server does indeed send this error type while developing this PR? Any steps to recreate the situation?

What I've tried so far was to make the backend (in my sandbox) fail the suspicious_email_login() call but the response I get on the client (WPAndroid) is this one:

{
	"error": "invalid_request",
	"error_description": "Please log in using your WordPress.com username instead of your email address."
}

Notice that the error is invalid_request, not email_login_not_allowed.

Any clues appreciated :) cc @aerych

@hypest
Copy link

hypest commented Apr 10, 2019

I tried out WPiOS (v12.0) against my sandbox and verified that if I rig the sandbox to always fail suspicious_email_login() (returning true) then the same response comes through Charles (Proxy) to the app. So, at this point, I'm not sure whether the issue is really fixed by this PR 🤔.

@mindgraffiti
Copy link
Contributor

mindgraffiti commented Apr 12, 2019

The wp.com changes have taken place: p1555065019002100-slack-C04B0MQUM. (Thank you @hypest!)

Can confirm, on the WCiOS side, I can receive the error: email_login_not_allowed and the authenticator reacts appropriately. Demo:

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Reviewers

2 more reviewers

@nheagy nheagy nheagy approved these changes

@mindgraffiti mindgraffiti mindgraffiti approved these changes

Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

enhancement New feature or request

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

5 participants

@aerych @hypest @mindgraffiti @nheagy